Single-source the version from pyproject.toml - #16
Merged
Conversation
The version was declared twice, in pyproject.toml and as a literal __version__ in python/__init__.py, so a release required editing both and either edit alone would leave the two disagreeing. Read it from the installed distribution metadata instead, leaving the [project] table in pyproject.toml as the single declaration. The alternative direction -- pyproject.toml declaring `dynamic = ["version"]` and reading `attr = "sbd.__version__"` -- was not taken. It relies on setuptools statically AST-parsing the assignment, and if setuptools ever falls back to importing the package it would import it before the extension modules are built, since this __init__ loads the _core_* backends at import time.
Jim Garrison (garrison)
marked this pull request as ready for review
September 2, 2026 16:57
Sophia Wen (hfwen0502)
added a commit
that referenced
this pull request
Sep 11, 2026
Squashes the 22 commits of fix-conda-build-and-mpi-detection, rebased onto main so that #15, #16, #17 and #21 are preserved rather than reverted. setup.py - Derive MPI_HOME from the MPI mpi4py is actually linked against, instead of guessing, so the extension and mpi4py cannot disagree at runtime. - Make conda-based builds work: strip the gcc-only tokens RHEL 9 CPython bakes into sysconfig that nvc++ rejects, and drop conda's -B compiler_compat while keeping its -L/-rpath entries. - Make SBD_GPU_ARCH optional and honor it at link as well as compile, so a multi-arch Thrust fatbin keeps every architecture it was asked for. This supersedes the narrower fix in #17. python/__init__.py, device_config.py - Load the backends lazily, one per process, and build all three by default. Removes the silent GPU demotion and the need for a second install. - Report why a backend is missing, and fail instead of silently using the CPU when gpu-omp has no device. README: rewritten around the conda recipe and split GPU prerequisites by build vs run. Retains #15's point that the sdist bundles the headers.
Sophia Wen (hfwen0502)
added a commit
that referenced
this pull request
Sep 11, 2026
* setup: derive MPI from mpi4py, fix conda builds, load backends lazily Squashes the 22 commits of fix-conda-build-and-mpi-detection, rebased onto main so that #15, #16, #17 and #21 are preserved rather than reverted. setup.py - Derive MPI_HOME from the MPI mpi4py is actually linked against, instead of guessing, so the extension and mpi4py cannot disagree at runtime. - Make conda-based builds work: strip the gcc-only tokens RHEL 9 CPython bakes into sysconfig that nvc++ rejects, and drop conda's -B compiler_compat while keeping its -L/-rpath entries. - Make SBD_GPU_ARCH optional and honor it at link as well as compile, so a multi-arch Thrust fatbin keeps every architecture it was asked for. This supersedes the narrower fix in #17. python/__init__.py, device_config.py - Load the backends lazily, one per process, and build all three by default. Removes the silent GPU demotion and the need for a second install. - Report why a backend is missing, and fail instead of silently using the CPU when gpu-omp has no device. README: rewritten around the conda recipe and split GPU prerequisites by build vs run. Retains #15's point that the sdist bundles the headers. * Add AMD GPU support to the OpenMP target-offload backend Squashes the four commits of amd-rocm-omp-offload. The same _core_gpu_omp_offload source now builds under either vendor: nvc++ -mp=gpu for NVIDIA, amdclang++ --offload-arch=gfx* for AMD. Thrust stays NVIDIA-only, since upstream wires it to nvc++ -cuda and there is no rocThrust configuration to build; SBD_BUILD_BACKEND=gpu therefore fails on AMD rather than silently falling back to the CPU. setup.py - detect_gpu_toolchain() picks nvidia or amd, with SBD_GPU_VENDOR to force the choice on a host carrying both; find_rocm_toolchain() honors ROCM_HOME then PATH, mirroring find_nvidia_hpc_sdk(). - SBD_GPU_ARCH is spelled per vendor and applied at compile and link: nvc++ takes -gpu=cc80,cc90 as one flag, clang needs a repeated --offload-arch=. Unset on AMD it is detected with the amdgpu-arch that ships beside the chosen compiler (not one found on PATH, which can come from a different ROCm), and a GPU-less build host is a hard error rather than an unrunnable module. - amdclang++ needs far less sysconfig scrubbing than nvc++: only -fcf-protection, and -march=x86-64-v2 is deliberately left alone. python/ - 'gpu-omp' is vendor-neutral; get_backend('gpu-omp').__sbd_offload_target__ reports what an install was actually built for, and the Thrust backend is stamped the same way. rocm/gpu-amd-omp/gpu-rocm-omp are aliases. - device_config.auto() resolves against the backends actually COMPILED instead of returning 'gpu' whenever any GPU was detected, which selected the CUDA Thrust backend on AMD hosts. GPU presence is probed with rocm-smi as well as nvidia-smi. - bindings.cpp pins each rank to one offload device, reading the vendor's device-visibility variable when omp_get_num_devices() reports 0 under dlopen. README: one rule for MPI -- every GPU backend hands MPI device pointers, so use a GPU-aware MPI (CUDA-aware on NVIDIA, ROCm-aware on AMD). Verified on MI250X (gfx90a, ROCm): 8 ranks, 4x2 grid, --device gpu-omp. * Pin the offload device in gdb_diag too sbd_pin_offload_device() was wired into tpb_diag and tpb_diag_from_files but not gdb_diag, so with --device gpu-omp every rank ran gdb_diag on the default device -- the "all ranks land on GPU 0" symptom, for that entry point only. Not caught earlier because the three call sites are copies rather than one helper; folding them together is a separate cleanup. * Tighten GPU detection and honor ROCM_PATH - Per-probe timeouts in _gpu_available(): rocm-smi keeps the 30 s it needs to enumerate a multi-GCD node, but nvidia-smi no longer inherits it. A wedged NVIDIA driver hangs rather than failing, and it was stalling backend selection for 30 s instead of 5. - Require a GPU[<n>] line from `rocm-smi --showid` rather than trusting its exit status, in both _gpu_available() and DeviceConfig._check_hip(). A ROCm install on a GPU-less BUILD host is exactly where a bare `rocm-smi` exiting 0 claims hardware that is not there, after which auto() picks a backend that cannot run. This is the same signal get_device_info() already counts devices with, so there is now one convention instead of two. - find_rocm_toolchain() honors ROCM_PATH as well as ROCM_HOME, checked in that order so an explicit ROCM_HOME still wins. ROCM_PATH is the variable ROCm itself sets, and tox.ini already passed it through to a build that ignored it. * macOS: take libomp and OpenBLAS from the conda env The Darwin block hardcoded Homebrew paths, so a conda env carrying llvm-openmp still compiled against /opt/homebrew -- while at import time the process loads the conda copies anyway, resolved through the interpreter's rpath. Prefer $CONDA_PREFIX when it has include/omp.h; Homebrew stays as the fallback. Two diagnostics, since this was hard to see: - fail with the fix when neither source has libomp, rather than "'omp.h' file not found" from the middle of a compile - print the resolved compiler and its version. sysconfig records a bare clang++, which PATH resolves, so a Homebrew LLVM silently wins over both Apple clang and a conda toolchain. README: llvm-openmp in both conda recipes, a note to pin CC/CXX, and a troubleshooting entry for a conda compiler package shadowing nvc++/amdclang++. Verified on macOS arm64 under Apple clang 21.0.0 and conda clang 23.1.0: no Homebrew references in the linked extension, pytest and the 2-rank MPI test green in both environments. * Restore the mpicc fallback for MPI detection Deriving the MPI prefix from mpi4py dropped main's mpicc probe, which broke CI: Debian splits Open MPI's headers to /usr/lib/<triple>/openmpi/include, outside the prefix, so mpi4py names the right MPI but $prefix/include/mpi.h does not exist and the build gave up. A prefix without mpi.h is an unusual layout, not the wrong MPI, so ask mpicc before failing -- both when the prefix lacks the header and when no prefix is found at all. mpi4py stays the preferred source, so the extension and mpi4py still cannot diverge. * Make the mpicc probe work for MPICH as well as Open MPI --showme:compile is an Open MPI-ism. MPICH's wrapper treats it as a source file and tries to compile it (exit 127 here), so probing that first silently ruled out MPICH -- which this package claims to support. Probe `-show` first, which both wrappers understand and which prints the whole command line, and keep --showme as the second attempt. Duplicate -I/-L entries are collapsed (MPICH repeats them), and the libraries the wrapper names are used as-is: -lmpi for Open MPI, -lmpi -lpmpi for MPICH. Verified against conda-forge openmpi 5.0.10 and mpich 5.0.1. * Probe mpicc with -show only -show is understood by every wrapper checked -- homebrew open-mpi, conda-forge openmpi 5.0.10, conda-forge mpich 5.0.1 -- while --showme:compile works on the Open MPI ones and exits 127 on MPICH. So -show strictly dominates, and the second attempt was dead weight: one subprocess call instead of up to three.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR was generated by Claude Opus 5 under my guidance.
The version was declared twice — once in
pyproject.tomland again as a literal__version__inpython/__init__.py. A release required editing both, and either edit alone would leave the two disagreeing with no error to signal it.This reads the version from the installed distribution metadata instead, leaving the
[project]table inpyproject.tomlas the single declaration:Note the argument is the distribution name (
sbd-eigensolver), not the import name (sbd).Why this direction
The other way to de-duplicate is to have
pyproject.tomldeclaredynamic = ["version"]and readattr = "sbd.__version__". That was not taken here. It depends on setuptools statically AST-parsing the assignment; if setuptools ever falls back to importing the package to resolve the attribute, it would importsbdbefore the extension modules are built, and this__init__loads the_core_*backends at import time. Deriving the runtime attribute from the metadata avoids any build-time import of the package.What this gives up
sbd.__version__now requires the distribution metadata to be present. That holds for bothpip install .andpip install -e .. It would break only for a source tree placed onsys.pathwithout being installed — which is not a working configuration for this package regardless, since the compiled_core_*modules must be built into the package directory; importing in that state already fails withRuntimeError: No SBD backends available.A
try/except PackageNotFoundErrorfallback was deliberately omitted: it would convert a loud, accurate failure into a silently wrong version string for a case that cannot arise in a working install.Verification
Checked in a throwaway virtualenv against a minimal package reproducing the same name mismatch and layout (
packages = ["sbd"],package-dir = {sbd = "python"}):pip install .→sbd.__version__ == "1.6.1"pip install -e .→sbd.__version__ == "1.6.1"pyproject.tomlto9.9.9and reinstalling →sbd.__version__ == "9.9.9", confirming the value tracks the single declaration with no second edittest/conftest.pyalready readssbd.__version__to build the pytest header, so the existing suite exercises this path on every run.